Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add DensifyGeodesic trait #1208

Closed
wants to merge 12 commits into from
Closed

Add DensifyGeodesic trait #1208

wants to merge 12 commits into from

Conversation

grevgeny
Copy link
Contributor

@grevgeny grevgeny commented Aug 10, 2024

  • I agree to follow the project's code of conduct.
  • I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

This PR adds DensifyGeodesic trait that follows logic similar to DensifyHaversine. However, the trait is only implemented for T = f64 as underlying Geodesic traits are implemented only for this type.

Copy link
Member

@michaelkirk michaelkirk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some minor nits, but overall it looks good. Thank you!

geo/src/algorithm/densify_geodesic.rs Outdated Show resolved Hide resolved
geo/src/algorithm/densify_geodesic.rs Show resolved Hide resolved
geo/src/algorithm/mod.rs Outdated Show resolved Hide resolved
@grevgeny
Copy link
Contributor Author

@michaelkirk thanks for your review! I have added proposed changes

@michaelkirk
Copy link
Member

I think you'll need to run cargo fmt, otherwise LGTM

I'll leave it open for a couple days before merging in case anyone else wants to weigh in.

@grevgeny
Copy link
Contributor Author

@michaelkirk

I've just realised that there is already the following method: geodesic_intermediate_fill. It expresses pretty much the same logic.

Do we still need this another implementation?

@michaelkirk
Copy link
Member

Darn it. I'm sorry for not realizing this earlier. This seems related to #1181

@michaelkirk
Copy link
Member

Assuming #1216 gets merged, I've got a different approach that should supersede this, generically implementing Densify for all line measures (Haversine, Geodesic, Euclidean, etc.)

You can see a WIP here: https://github.com/georust/geo/compare/mkirk/unify-densify?expand=1

@michaelkirk
Copy link
Member

michaelkirk commented Oct 23, 2024

My alternative to this was merged in #1231.

With that you can call:

use geo::{Geodesic, Densify, Euclidean};
line_string.densify::<Geodesic>();
line_string.densify::<Euclidean>();

Let me know if that works for your purposes @grevgeny.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants